Skip to content

fix(mcp): fix resource leak and stale-header bug in MCP client - #890

Open
h4sht wants to merge 1 commit into
CodebuffAI:mainfrom
h4sht:fix/mcp-client-resource-leak
Open

fix(mcp): fix resource leak and stale-header bug in MCP client#890
h4sht wants to merge 1 commit into
CodebuffAI:mainfrom
h4sht:fix/mcp-client-resource-leak

Conversation

@h4sht

@h4sht h4sht commented Jul 22, 2026

Copy link
Copy Markdown

Summary

Fixes two related bugs in the MCP client module that cause resource leaks and stale connections:

1. Memory/Connection Leak — No Client Cleanup

The `runningClients` map and `listToolsCache` in `common/src/mcp/client.ts` grew without bound. Once an MCP client was connected, it was never removed from the registry, even after the transport died or the session ended. In long-running sessions with MCP tools, this accumulates stale connections and never releases resources.

Fix: Added `closeMCPClient(clientId)` — closes the underlying transport (stdio process or HTTP/SSE connection) and removes entries from both `runningClients` and `listToolsCache`.

2. Stale Headers in Config Hash

`hashConfig()` did not include `config.headers` when computing the cache key for HTTP and SSE transports. Two MCP server configs with different auth tokens but the same URL were treated as the same client, returning a cached client with stale credentials.

Fix: Added `headers: config.headers` to the hash for both `http` and `sse` config types.

3. Automatic Cleanup on Tool Call Failure

When an MCP tool call failed (broken connection, dead stdio process), the dead client remained in the cache. Subsequent calls would reuse the same broken connection and fail with the same error.

Fix: In `sdk/src/run.ts`, the MCP tool call catch block now calls `closeMCPClient()` to evict the dead client so the next call reconnects with a fresh transport.

Files changed

  • `common/src/mcp/client.ts` — Fixed `hashConfig`, added `closeMCPClient` export
  • `sdk/src/run.ts` — Imported `closeMCPClient` and integrated into MCP error handling

Known limitations

  • `closeMCPClient` only evicts clients reactively (on tool call failure). There is no proactive liveness check in `getMCPClient` for transports that die silently (e.g., stdio process exits without a pending call). This means a stale entry could serve one bad request before being evicted. A follow-up could add a health check or TTL-based eviction.

tru3
fix(mcp): fix resource leak and stale-header bug in MCP client
62cc7eb 
- Fix hashConfig to include headers for HTTP/SSE configs, preventing
  stale auth tokens when two MCP servers differ only by headers.

- Add closeMCPClient() export to clean up client connections and
  cached tool listings, fixing the memory/connection leak where
  runningClients and listToolsCache grew without bound.

- Integrate automatic cleanup in SDK: when an MCP tool call fails,
  the dead client is evicted so the next call reconnects with a
  fresh transport instead of reusing the broken connection.
@chrismindpower369

chrismindpower369 commented Aug 12, 2026

Copy link
Copy Markdown

Thanks — this addresses two important reliability issues for projects using authenticated HTTP/SSE MCP servers.

Including config.headers in the cache key is especially important: otherwise switching tokens or credentials for the same endpoint can silently reuse the wrong client. Evicting a client after a failed tool call also gives the next request a clean reconnect path.

Would it be possible to add regression tests for:

  • same endpoint with different headers creates distinct clients
  • a failed MCP tool call removes both the client and its listToolsCache entry
  • closeMCPClient() safely handles an already-closed or missing client

The documented limitation around silently dead transports also makes sense. A small follow-up for a liveness check or TTL-based eviction would improve long-running sessions further. Overall, this looks like a valuable fix for real user projects.

@codebuff-team

codebuff-team commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Good catch on both bugs. The hashConfig fix (including headers for http/sse) is a clear, correct fix for a real staleness bug — two configs differing only by auth header would otherwise collide on the same cached client, which is a legitimate correctness issue worth porting as-is.

The closeMCPClient addition and its use in sdk/src/run.ts's catch block are reasonable, though worth double-checking on the maintainer's side:

  • Eviction is reactive only (as you note in "Known limitations") — a client that dies silently between calls (e.g., stdio process exits without a pending request) will still serve one failed call before eviction. That's an acceptable tradeoff for a small fix, not a blocker.
  • closeMCPClient swallows all errors from client.close(), which is probably fine given the transport may be dead, but it also means genuine close-failures (e.g., a hung process) are invisible. Consider at least a debug log if the codebase has a logger convention.
  • No tests are included. Given the size of the change, a unit test for hashConfig distinguishing two configs by headers, and for closeMCPClient no-oping on unknown ids, would make this easier for a maintainer to accept without re-deriving the fix.

Overall this is a small, well-targeted fix addressing a real caching bug with a clear root cause. Recommend porting after maintainers verify test coverage or add a couple of unit tests. Nice first PR.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@h4sht @chrismindpower369 @codebuff-team